Rebase - #48
Conversation
…ts To C Implementation (#35) * Created a comprehensive L2 test suite with 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies Updated build scripts and CI/CD workflow to compile with L2_TEST_ENABLED flag and execute the new test suite This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive L2 (Level 2) test suite for the uploadSTBLogs service, consisting of 6 test modules covering error handling, normal uploads, retry logic, security, resource management, and upload strategies. The changes include conditionally disabling sleep() calls in the reboot strategy implementation when testing is enabled, and updates to build scripts and CI/CD workflows to support the new test infrastructure.
Key Changes
- Conditional compilation of sleep() calls in strategy_reboot.c using L2_TEST_ENABLED flag to speed up testing
- New test infrastructure with Python-based test suites and Gherkin feature files for behavior-driven testing
- Build configuration updates to include L2_TEST_ENABLED flag and reference the correct branch
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 82 comments.
Show a summary per file
| File | Description |
|---|---|
| uploadstblogs/src/strategy_reboot.c | Wraps sleep() calls with #ifndef L2_TEST_ENABLED to allow faster test execution |
| test/run_uploadstblogs_l2.sh | Test runner script that executes all 6 L2 test suites with proper environment setup |
| test/functional-tests/tests/uploadstblogs_helper.py | Helper functions for test execution including log parsing, process management, and file operations |
| test/functional-tests/tests/test_uploadstblogs_*.py | Six comprehensive test modules covering different aspects of the uploadSTBLogs service |
| test/functional-tests/features/*.feature | Gherkin feature files describing test scenarios in behavior-driven format |
| cov_build.sh | Updated to use feature/upload_L2 branch and add L2_TEST_ENABLED compiler flag |
| .github/workflows/L2-tests.yml | Updated CI/CD workflow to execute the new L2 test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = subprocess.run(f"grep '{marker}' /opt/logs/telemetry*.log 2>/dev/null", | ||
| shell=True, capture_output=True, text=True) |
There was a problem hiding this comment.
Using shell=True with subprocess.run poses a security risk when the marker parameter contains unsanitized input. The marker parameter is directly interpolated into a shell command. Consider using a list of arguments without shell=True or properly sanitizing the marker parameter.
| result = subprocess.run(f"grep -c '{pattern}' {log_file} 2>/dev/null || echo 0", | ||
| shell=True, capture_output=True, text=True) | ||
| return int(result.stdout.strip()) |
There was a problem hiding this comment.
Using shell=True with subprocess.run poses a security risk when the pattern parameter contains unsanitized input. The pattern parameter is directly interpolated into a grep command. Consider validating/sanitizing the pattern parameter or using a Python-based search instead of grep.
| result = subprocess.run(f"grep -c '{pattern}' {log_file} 2>/dev/null || echo 0", | |
| shell=True, capture_output=True, text=True) | |
| return int(result.stdout.strip()) | |
| try: | |
| count = 0 | |
| with open(log_file, "r", encoding="utf-8", errors="ignore") as f: | |
| for line in f: | |
| if pattern in line: | |
| count += 1 | |
| return count | |
| except (FileNotFoundError, PermissionError, OSError): | |
| return 0 |
| # Check for archive creation logs | ||
| archive_logs = grep_uploadstb_logs_regex(r"Archive created successfully") | ||
| # Process should complete successfully | ||
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" |
There was a problem hiding this comment.
The assertion message contains an f-string that is not properly formatted. The message should use an f-string prefix to include the variable values, i.e., f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}"
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | |
| assert len(archive_logs) > 0, f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" |
| @@ -0,0 +1,70 @@ | |||
| #################################################################################### | |||
| # If not stated otherwise in this file or this component's Licenses | |||
There was a problem hiding this comment.
The comment header has a typo. It should say "Licenses.txt file" instead of just "Licenses file". This inconsistency appears across multiple files in this PR and should be corrected for consistency with standard RDK copyright headers.
| # If not stated otherwise in this file or this component's Licenses | |
| # If not stated otherwise in this file or this component's Licenses.txt file |
| cd telemetry | ||
| cp include/*.h /usr/local/include | ||
| sh build_inside_container.sh | ||
| sh build_inside_container.sh |
There was a problem hiding this comment.
This line has an unnecessary trailing space after the shell command. The space should be removed for cleaner code.
| sh build_inside_container.sh | |
| sh build_inside_container.sh |
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import os | ||
| import time | ||
| import re | ||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
| """Get file size in bytes""" | ||
| try: | ||
| return os.path.getsize(filepath) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| cd /usr/common_utilities | ||
| sed -i '/file_upload\.sslverify/s/= 1;/= 0;/' uploadutils/mtls_upload.c | ||
| sed -i 's/\(ret_code = setCommonCurlOpt(curl, s3url, NULL, \)true\()\)/\1false\2/g' uploadutils/uploadUtil.c | ||
| sed -i '/if (auth) {/,/}/s/^/\/\/ /' uploadutils/uploadUtil.c |
There was a problem hiding this comment.
The test runner script directly patches /usr/common_utilities/uploadutils/mtls_upload.c and uploadutils/uploadUtil.c to disable TLS certificate verification and bypass authentication by forcing file_upload.sslverify to 0, changing setCommonCurlOpt(..., true) to false, and commenting out the if (auth) { ... } block. If this script is run in any environment whose binaries are then used beyond isolated testing, it produces builds that accept unverified HTTPS servers and skip authentication, enabling man-in-the-middle attacks and unauthorized log uploads. Instead of source-level sed patches that weaken security controls, introduce test-specific configuration or compile-time flags that preserve proper certificate validation and authentication in all non-test builds.
…ts To C Implementation (#49) * Simplify some pieces of code over engineered by AI models. * Update run_uploadstblogs_l2.sh for L2 coverage --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>
…ts To C Implementation (#51) * Remove logging of response URLs * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Shibu Kakkoth Vayalambron <shibu.kakkoth@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| result = run_uploadstblogs() | ||
|
|
||
| # Check for configuration error detection | ||
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") |
There was a problem hiding this comment.
Variable error_logs is not used.
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") | |
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") | |
| assert len(error_logs) > 0, "Configuration error should be logged for corrupted device properties" |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for informative message | ||
| logs = grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload") |
There was a problem hiding this comment.
Variable logs is not used.
| logs = grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload") | |
| grep_uploadstb_logs_regex(r"no.*file|empty|nothing to upload") |
| result = run_uploadstblogs() | ||
|
|
||
| # Should not attempt upload | ||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") |
There was a problem hiding this comment.
Variable upload_logs is not used.
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| assert len(upload_logs) == 0, "Should not log upload attempts without files" |
…ts To C Implementation (#54) * Update cov_build.sh to enable the rdkcertselector feature in the common_utilities build configuration to support the migration of operation support log upload scripts to C implementation. The change is part of resolving L2 test failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 29 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = run_uploadstblogs() | ||
|
|
||
| # Check for configuration error detection | ||
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") |
There was a problem hiding this comment.
Variable error_logs is not used.
| error_logs = grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") | |
| grep_uploadstb_logs_regex(r"ERROR|error|fail.*properties|invalid") |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for size-related logs | ||
| logs = grep_uploadstb_logs_regex(r"size|large|limit|truncate") |
There was a problem hiding this comment.
Variable logs is not used.
| logs = grep_uploadstb_logs_regex(r"size|large|limit|truncate") | |
| grep_uploadstb_logs_regex(r"size|large|limit|truncate") |
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | ||
|
|
||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | ||
| # Telemetry should be attempted | ||
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" |
There was a problem hiding this comment.
Variable upload_logs is not used.
| assert len(archive_logs) > 0, "Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | |
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| # Telemetry should be attempted | |
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" | |
| assert len(archive_logs) > 0, ( | |
| f"Archive process should complete. Found {len(archive_logs)} archive-related logs: {archive_logs}" | |
| ) | |
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | |
| # Telemetry should be attempted | |
| assert len(upload_logs) > 0, ( | |
| f"Upload process should complete and succeed. Found {len(upload_logs)} upload-related logs: {upload_logs}" | |
| ) |
| # Check for compression/archive activity | ||
| compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz") |
There was a problem hiding this comment.
Variable compression_logs is not used.
| # Check for compression/archive activity | |
| compression_logs = grep_uploadstb_logs_regex(r"compress|archive|tgz") |
| result = run_uploadstblogs() | ||
|
|
||
| # Check for no files detection | ||
| no_files_logs = grep_uploadstb_logs_regex(r"no.*file|empty|not found|no logs") |
There was a problem hiding this comment.
Variable no_files_logs is not used.
| result = run_uploadstblogs() | ||
|
|
||
| # Check for event publishing | ||
| event_logs = grep_uploadstb_logs_regex(r"event|publish|success") |
There was a problem hiding this comment.
Variable event_logs is not used.
| event_logs = grep_uploadstb_logs_regex(r"event|publish|success") | |
| grep_uploadstb_logs_regex(r"event|publish|success") |
| result = run_uploadstblogs(args) | ||
|
|
||
| # Check strategy logging | ||
| logs = grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type") |
There was a problem hiding this comment.
Variable logs is not used.
| logs = grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type") | |
| grep_uploadstb_logs_regex(r"strategy|STRAT_|upload.*type") |
| try: | ||
| subprocess.run(f"echo '' > {UPLOADSTB_LOG}", shell=True) | ||
| return True | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| if os.path.exists(LOCK_FILE): | ||
| os.remove(LOCK_FILE) | ||
| return True | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| for chunk in iter(lambda: f.read(4096), b""): | ||
| md5_hash.update(chunk) | ||
| return md5_hash.hexdigest() | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 63 changed files in this pull request and generated 22 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = run_uploadstblogs() | ||
|
|
||
| # Check for size limit detection | ||
| size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") |
There was a problem hiding this comment.
Variable size_logs is not used.
| size_logs = grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") | |
| grep_uploadstb_logs_regex(r"size.*limit|too large|exceed") |
|
|
||
| # Check archives before | ||
| archives_before = sp.run("ls /tmp/*.tgz 2>/dev/null | wc -l", | ||
| shell=True, capture_output=True, text=True) | ||
|
|
||
| result = run_uploadstblogs() | ||
| time.sleep(2) | ||
|
|
There was a problem hiding this comment.
Variable archives_before is not used.
| # Check archives before | |
| archives_before = sp.run("ls /tmp/*.tgz 2>/dev/null | wc -l", | |
| shell=True, capture_output=True, text=True) | |
| result = run_uploadstblogs() | |
| time.sleep(2) | |
| result = run_uploadstblogs() | |
| time.sleep(2) |
|
|
||
| upload_logs = grep_uploadstb_logs_regex(r"upload.*success|uploading|HTTP") | ||
| # Telemetry should be attempted | ||
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" |
There was a problem hiding this comment.
Variable upload_logs is not used.
| assert len(archive_logs) > 0, "Upload Process should complete and succeed" | |
| assert len(upload_logs) > 0, "Upload Process should complete and succeed" |
| proc = sp.Popen([UPLOADSTB_BINARY], stdout=sp.PIPE, stderr=sp.PIPE) | ||
| time.sleep(2) | ||
|
|
||
| memory_during = check_memory_usage("uploadSTBLogs") |
There was a problem hiding this comment.
Variable memory_during is not used.
| memory_during = check_memory_usage("uploadSTBLogs") | |
| memory_during = check_memory_usage("uploadSTBLogs") | |
| assert memory_during > 0, "Process should use memory while running" |
|
|
||
| start_time = time.time() | ||
| result = run_uploadstblogs() | ||
| elapsed = time.time() - start_time |
There was a problem hiding this comment.
Variable elapsed is not used.
| elapsed = time.time() - start_time | |
| elapsed = time.time() - start_time | |
| assert elapsed >= 0.0, "Execution time should be measured" |
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| """ | ||
|
|
||
| import pytest | ||
| import time |
There was a problem hiding this comment.
Import of 'time' is not used.
| import time |
| import os | ||
| import time | ||
| import re | ||
| import json |
There was a problem hiding this comment.
Import of 'json' is not used.
| import json |
| """Get file size in bytes""" | ||
| try: | ||
| return os.path.getsize(filepath) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except: | |
| except OSError: |
DCM Agent 2.0.1 release
* Create uploadlogsnow.h --------- Co-authored-by: nhanasi <navihansi@gmail.com>
* Update L2-tests.yml * L2 automatics upload Error --------- Co-authored-by: Abhinav P V <Abhinav_Valappil@comcast.com>
…161) * Update dcm_parseconf.c * Update log message for backup_logs sentinel detection * Update backup_engine.c * Remove old copyright and documentation comments
* Update archive_manager.h * Update archive_manager.c * Update strategies.c * Update archive_manager.c * Update archive_manager.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Update strategies.c * Fix formatting of comment in strategies.c * Update archive_manager_gtest.cpp * Update strategies_gtest.cpp
…from RDK analytics portal (#169) * Update strategies.c * Update strategies.c * Remove unnecessary blank line in strategies.c
…ed to logupload server is not available (#168) * Update archive_manager.c * Update backup_engine.c * Update backup_engine_gtest.cpp * Update backup_engine_gtest.cpp * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update backup_logs_gtest.cpp * Update backup_engine_gtest.cpp * Update backup_logs_gtest.cpp * Update Makefile.am * Update backup_engine_gtest.cpp * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update Makefile.am * Update backup_engine_gtest.cpp * Update backup_engine.c * Clean up mock functions and tests for backup logs Removed unused mock open functions and related tests for backup_and_recover_logs. * Remove -U_FORTIFY_SOURCE from backup_engine_gtest_CFLAGS
…ts To C Implementation (#35)
This feature file contains scenarios for normal log uploads, large log file handling, and MD5 checksum verification for the uploadSTBLogs service.